perf(profiling): consolidate greenlet switch updates - #19655
Conversation
Codeowners resolved asResolved from the full PR diff against No remaining files require a CODEOWNERS review. |
Circular import analysis
|
Dependency direction analysis
|
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 1 job - 1 passed on retry 🔗 Commit SHA: 9370289 | Docs | Datadog PR Page | Give us feedback! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Consolidates greenlet switch frame updates into one native call and mutex acquisition to reduce profiling overhead.
Changes:
- Adds combined Python/native greenlet-switch update APIs.
- Preserves conditional parent-frame retention.
- Adds forwarding coverage and updates related documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ddtrace/profiling/_gevent.py |
Consolidates switch updates. |
stack.cpp |
Exposes the native switch API. |
sampler.cpp |
Updates both frames under one lock. |
sampler.hpp |
Declares the combined method. |
threads.cc |
Updates lock-contention commentary. |
_stack.pyi |
Updates native API typing. |
__init__.pyi |
Updates exported API typing. |
test_gevent.py |
Adds forwarding regression coverage. |
test_stack.py |
Updates contention-test documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
KowalskiThomas
left a comment
There was a problem hiding this comment.
Very cool find and change.
df0dc65
into
main
Description
Consolidates the two native frame updates performed for each greenlet switch into one private
record_greenlet_switch()call and onegreenlet_info_mapmutex acquisition.The existing behavior is preserved: the origin frame is always updated, while the running target frame is left unchanged when it is needed for parent-greenlet stack unwinding.
This is an independently mergeable prerequisite extracted from #18724.
Testing
scripts/lint fmt -- ddtrace/profiling/_gevent.py tests/profiling/test_gevent.py tests/profiling/collector/test_stack.pyscripts/lint cformatscripts/lint profiling-native-checkscripts/lint checksgit diff --checkBenchmark
The benchmark isolates the native operation changed by this PR. Each measured run performs 1,000,000 logical greenlet switch updates after 10,000 warm-up updates:
e11d640110: twoupdate_greenlet_frame()native calls and two mutex acquisitions per update93702898f8: onerecord_greenlet_switch()native call and one mutex acquisition per updateBoth
_stackextensions were built in Release mode from the exact commits, then run in the same container pinned to CPU 0 with--cpuset-cpus=0.Results over five runs:
[0.528339, 0.531681, 0.532128, 0.534566, 0.529234][0.380415, 0.368066, 0.360803, 0.362898, 0.366206]Environment:
sha256:ce7c46dbb7f07d352aecd756e19fd7f39550a72d4a13e68d30f02f5448eebd69Benchmark script
This is an isolated native-call benchmark, not an end-to-end application throughput claim. The earlier 67% figure was discarded after reproducing with exact baseline and candidate builds because the candidate run had not activated the profiling hook.
Risks
Low. The changed Python and native APIs are private and ship together. Existing parent-greenlet frame retention remains unchanged.
Additional Notes
The GIL release around native state mutation is retained intentionally. It was introduced by #14852 in commit
54a3a0ea35to prevent potential thread-pool deadlocks while waiting for profiler mutexes. This change reduces two such release windows per greenlet switch to one.No release note is needed because this is an internal performance optimization with no user-facing API or behavior change.